fix(hugegraph-dist): gate init-store on a dedicated init_store.enabled option - #3119
fix(hugegraph-dist): gate init-store on a dedicated init_store.enabled option#3119bitflicker64 wants to merge 19 commits into
Conversation
…g is false Honor the existing ServerOptions flag in InitStore so distributed/HStore can opt out of local backend and admin init. Unset keeps full init for standalone. Docker maps HG_SERVER_LOAD_FROM_LOCAL_CONFIG to rest-server.properties. Fixes apache#3118
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Disabling local graph loading also bypasses the Docker password-based admin bootstrap, so the requested credential is never installed. Evidence: exact-head static trace through docker-entrypoint.sh, InitStore.java, and GraphManager.java; 3/3 focused tests passed; visible checks are green.
…d option Replace the graph.load_from_local_config gate with a dedicated init_store.enabled option defaulting to true. That property already means whether GraphManager loads graph definitions from the local directory, and existing configs may materialize its declared false default, so reusing it made an explicit false skip backend and admin initialization for standalone RocksDB/HBase installs. The early return also skipped StandardAuthenticator.initAdminUserIfNeeded, so a Docker PASSWORD was piped into init-store.sh and discarded without creating the admin, leaving the server on the auth.admin_pa default. In skip mode the entrypoint now writes PASSWORD to auth.admin_pa instead, and does not create docker/init_complete since nothing was initialized. The gate is read back from rest-server.properties so a config mounted without the env var behaves the same way. Cover the lifecycle with a main-level test whose graphs directory would fail if the early exit were missed, and entrypoint smoke tests for default init, PASSWORD, skip, skip with PASSWORD, mounted property, and false to true restart. Run those in Docker CI and extend its path filter to the dist docker directory.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3119 +/- ##
============================================
- Coverage 39.13% 32.41% -6.73%
Complexity 264 264
============================================
Files 770 771 +1
Lines 65772 65930 +158
Branches 8725 8762 +37
============================================
- Hits 25741 21368 -4373
- Misses 37272 42110 +4838
+ Partials 2759 2452 -307 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The skip path still fails admin bootstrap for the shipped HStore configuration and has inconsistent option/password handling; backend and plugin registration also occurs before the gate. Evidence: six independent review lanes, exact-head static traces, an uppercase FALSE regression test failing four assertions, and all visible latest-head checks passing.
… is true With init_store.enabled=false the built-in admin account is not created by init-store, and the only other component that creates it is GraphManager.initAdminUserIfNeeded(), reached from loadMetaFromPD() and so gated on usePD. That option defaults to false and the shipped HStore compose files do not set it, so enabling auth in skip mode would start a server that enforces authentication with no account to authenticate against. The entrypoint now refuses that combination and exits with an explanation, and InitStore logs the same condition for non-Docker installs. Skipping without auth is unaffected. usePD=true is already the documented pairing for distributed deployments in the cluster-test rest-server.properties template. Also align the shell with the server's boolean parsing, which is case-insensitive, so HG_SERVER_INIT_STORE_ENABLED=FALSE no longer means skip to Java and run to the entrypoint; read the property through the separators a properties file allows; escape the password for properties serialization so a backslash survives; and register backends and plugins only on the enabled path, since plugin registration runs every plugin's register() and propagates its failures. Add entrypoint coverage for these plus a test that disabled mode leaves backend options unregistered. Note a pre-existing arthas config key mismatch found while checking the option table.
|
Unrelated to this change, but noticed while checking the The arthas options are declared camelCase ( It is invisible today because each shipped value equals its default. It bites on any change, e.g. |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The mounted-properties override path can create duplicate scalar keys and make init-store or server startup fail. Evidence: six independent review lanes, exact-head static tracing through docker-entrypoint.sh and HugeConfig.java, and all visible latest-head checks passing.
get_prop was widened to read the `=`, `:` and whitespace separators a properties file allows, but set_prop still matched only `key=value`. A mounted `init_store.enabled:false` overridden by HG_SERVER_INIT_STORE_ENABLED therefore gained a second definition instead of being replaced, and a key defined twice collects both values into a list that fails the scalar type check while the config is still loading. Both init-store and server startup would fail. The same applied to a colon-form auth.admin_pa overridden by PASSWORD. set_prop now matches the same separators and collapses every existing definition into one canonical `key=value` line, leaving comments alone. It is written in awk, so the key and value are matched and emitted literally and no regex escaping is involved. Cover colon-form and whitespace-form overrides asserting a single remaining definition, a colon-form auth.admin_pa override, and that commented defaults are not treated as definitions. Add a test loading a duplicated key through HugeConfig to pin why the collapse is required.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The disabled init path can still report success for an unusable auth configuration, and two Docker config rewrite paths can fail or corrupt mounted/protected configs. Evidence: six independent exact-head review lanes; entrypoint smoke tests passed 57 assertions with 2 macOS-only skips; git diff --check passed; codecov/patch and codecov/project are failing.
…onf in place Four fixes to the skip path. InitStore returned zero for a configuration it had just reported as unusable: init_store.enabled=false with an authenticator set and usePD false means no component creates the built-in admin. Tarball and init-job callers see only the exit status, so they continued into an auth-enabled server nobody could reach. It now fails. Remote auth is exempt, since the auth manager is then an RPC client and StandardAuthenticator only bootstraps a local one; the entrypoint guard gains the same exemption, which it was missing. set_prop replaced the config by rename. A single-file bind mount cannot be replaced that way, so startup aborted under set -e, and the rename also discarded the original mode, which matters where auth.admin_pa is written. It now truncates and rewrites in place, and creates its scratch file under umask 077. On the skip path enable-auth.sh ran unconditionally, and it appends its keys without checking, so a mounted config that already enabled auth ended up with auth.authenticator and auth.graph_store defined twice, which the config parser rejects. It now runs only when auth is not configured yet, and both keys are collapsed to single definitions afterwards. The test guard that skipped two cases without GNU sed was stale once set_prop moved to awk. Removed, so those cases run everywhere. Cover the exit status and the remote-auth exemption in the CLI test, and add entrypoint cases for a mounted auth config, the remote-auth exemption, inode and mode preservation, and scratch file cleanup.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The skip path can leave Gremlin authentication incomplete and rejects supported custom authenticators. Evidence: six independent exact-head review lanes, static traces through enable-auth.sh and HugeAuthenticator.loadAuthenticator(), 74/74 entrypoint assertions passing, git diff --check passing, and all visible latest-head workflows passing.
…n check enable-auth.sh also adds the authentication block to gremlin-server.yaml and switches hugegraph.properties to the auth proxy. Skipping it whenever auth.authenticator already existed left both undone, so a mounted config that set only that key could start with REST auth on and Gremlin open. Apply each of the three changes only where it is missing instead, and point the Gremlin block at whichever authenticator the REST config names. The admin-account requirement now applies only to the built-in authenticator and its subclasses. auth.authenticator takes any implementation class, and a custom LDAP/OIDC/plugin one manages identities elsewhere, so requiring usePD for it rejected a deployment that works. get_prop no longer deletes whitespace inside a value, only around it, as the properties parser does.
41259fb to
0dd5a7a
Compare
- get_prop uses `sed -E` rather than `-r`, which only GNU sed documents - the appended gremlin-server.yaml block no longer glues itself onto a last line that has no trailing newline - the option description, the InitStore comment and docker/README no longer claim every authenticator needs the built-in admin account, which stopped being true when the check was narrowed - InitStoreConfigTest is imported in suite order in UnitTestSuite - a test asserts the entrypoint's inlined auth block still names everything bin/enable-auth.sh does, so the two cannot drift unnoticed
|
Did a self-review pass over the whole diff and pushed 090763a with the follow-ups, no behaviour change beyond the two fixes below:
The rest is text and hygiene: the option description, the Since the entrypoint reapplies what Local run: 106 assertions across 30 entrypoint cases, unit suite 626 tests green on JDK 11, rat and editorconfig clean. |
# Conflicts: # hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The skip path still has inconsistent admin-bootstrap decisions across Docker and Java, and several auth configurations can start unusable or incompletely protected. Evidence: six independent exact-head review lanes, static traces through docker-entrypoint.sh, InitStore, GraphManager, and StandardHugeGraph; 106/106 entrypoint assertions and the focused Java test passed; git diff --check passed; visible Actions passed but Codecov statuses failed.
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated init_store.enabled server option (default true) to gate InitStore’s local initialization behavior, enabling PD/HStore (distributed) deployments to skip local backend/admin initialization while still validating that authentication is usable. It also updates the Docker entrypoint and wrappers to apply the same decision consistently, with improved property-file handling and extensive lifecycle coverage.
Changes:
- Introduces
init_store.enabled(ServerOptions.INIT_STORE_ENABLED) and wires it intoInitStorewith a validation-only disabled path. - Enhances Docker entrypoint behavior for init-store gating, auth bootstrap/persistence, property canonicalization, and safer secret writes.
- Adds focused Java unit tests, entrypoint smoke tests, and CI wiring for the new entrypoint test suite.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java | Registers the new InitStore config unit tests in the unit suite. |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/cmd/InitStoreConfigTest.java | Adds unit tests covering init_store.enabled, admin password handling, and disabled-path validation rules. |
| hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/cmd/InitStore.java | Implements the init_store.enabled gate, disabled-path validation, and optional configured-password mode. |
| hugegraph-server/hugegraph-dist/src/assembly/static/bin/init-store.sh | Updates the init-store wrapper to accept/forward an optional flag and propagate Java exit codes. |
| hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh | Adds a comprehensive entrypoint lifecycle smoke test suite. |
| hugegraph-server/hugegraph-dist/docker/test/JavaPropertiesReader.java | Adds a helper to validate Java-properties parsing round-trips (including tricky UTF-8 / escapes). |
| hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh | Applies init-store gating + auth enablement completion, canonicalizes properties, and persists secrets safely when needed. |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java | Defines the new init_store.enabled configuration option. |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/StandardAuthenticator.java | Extends admin init to support configured-password mode and ensures auth graph is always closed. |
| docker/README.md | Documents HG_SERVER_INIT_STORE_ENABLED semantics and the auth-related constraints in disabled mode. |
| .github/workflows/docker-build-ci.yml | Adds a CI job to run the entrypoint smoke tests when Docker-related paths change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The latest head can create a predictable admin credential and still has a properties-parser gap that can leave authentication incomplete; three narrower cleanup and test-trigger issues also remain. Evidence: exact-head static traces through the entrypoint, InitStore, StandardAuthenticator, and workflow; visible Actions passed while Codecov patch/project failed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/StandardAuthenticator.java:240
- StandardAuthenticator.initAdminUserIfNeeded() only checks whether auth.authenticator is empty, but it doesn't verify that the configured authenticator is actually StandardAuthenticator (or a subclass). If this helper is invoked with a config that sets a custom/local authenticator, it can still open the auth graph and potentially create HugeGraph's built-in admin account even though that authenticator doesn't use it. Add a defensive check so the method no-ops unless auth.authenticator resolves to StandardAuthenticator (or subclass).
HugeConfig config = new HugeConfig(confFile);
String authClass = config.get(ServerOptions.AUTHENTICATOR);
if (authClass.isEmpty()) {
return;
}
config.addProperty(INITING_STORE, true);
auth.initAdminUser(config, password, fromConfig);
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The current head still has upgrade and authentication-consistency regressions, plus configuration durability and deployment-hardening gaps. Evidence: six independent exact-head review lanes; static traces through docker-entrypoint.sh, ConfigTool, and init-store.sh; 217/217 entrypoint assertions passed; Actions passed while codecov/patch and codecov/project failed.
| "$(get_prop "auth.graph_store" "${REST_SERVER_CONF}")") | ||
| STORED_AUTH_STATE=$(cat \ | ||
| "${DOCKER_FOLDER}/${AUTH_INIT_STATE_FILE}" 2>/dev/null || true) | ||
| if [[ -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" && |
There was a problem hiding this comment.
docker/init_complete and a working built-in admin but no new auth_init_state; the empty stored state then makes AUTH_INIT_REQUIRED=true, and lines 298-304 reject startup unless the operator supplies PASSWORD or a persisted auth.admin_pa. Previous authenticated initialization passed the password over stdin and did not persist it, so this turns a valid existing deployment into an unavailable one. Please distinguish the legacy/missing-state upgrade case and inspect whether admin bootstrap is actually needed before requiring a new secret; add a regression with init_complete, no auth_init_state, an existing admin, and no supplied password.
There was a problem hiding this comment.
The entrypoint auth bootstrap moved out of this PR to #3133. The upgrade case you describe is listed there as one of the shapes it has to handle — a legacy volume carrying init_complete without the bootstrap state the new code expects. Left unresolved so it stays findable.
| esac | ||
| } | ||
|
|
||
| gremlin_auth_configured() { |
There was a problem hiding this comment.
StandardAuthenticator to a custom authenticator, an existing Gremlin block is accepted unchanged, so old HugeGraph credentials can remain valid through Gremlin. It also misses valid YAML such as authentication : {...} and appends a duplicate block. Please parse and reconcile the effective top-level YAML authentication configuration with the selected REST authenticator (or fail closed on mismatch), and add transition plus separator-whitespace regressions.
There was a problem hiding this comment.
Moved to #3133, listed first there. Agreed the presence-only check cannot distinguish a partially configured file from a complete one, which is exactly the Gremlin/REST split you describe.
| FileHandler handler = new FileHandler(config); | ||
| handler.save(scratch.toFile()); | ||
| try (InputStream input = Files.newInputStream(scratch); | ||
| OutputStream output = Files.newOutputStream( |
There was a problem hiding this comment.
TRUNCATE_EXISTING leaves the target partial or empty, and the finally block then deletes the complete scratch copy; that can erase the persisted admin credential and make every restart fail. Please retain a recoverable copy until replacement succeeds, using atomic replacement for ordinary files and a guarded backup/restore path for bind mounts, with an injected mid-copy failure test.
There was a problem hiding this comment.
ConfigTool is out of this PR and moves to #3133 with the rest of the entrypoint work, so this is unaddressed rather than dismissed — write atomicity needs solving there before it lands again.
|
|
||
| static void setProperty(String file, String key, String value) | ||
| throws ConfigurationException, IOException { | ||
| PropertiesConfiguration config = load(file); |
There was a problem hiding this comment.
PropertiesConfiguration silently destroys Commons Configuration include semantics. include and includeoptional files are resolved into the effective configuration, but PropertiesConfigurationLayout documents that saving flattens included properties into the parent and drops the include directive. A single environment override can therefore stop later child-file updates from applying and copy included secrets into a broader-permission parent file. Please preserve include directives during mutation or reject rewriting files that use them, and add a parent/child include regression.
There was a problem hiding this comment.
Same — ConfigTool moves to #3133. The include-semantics point is the strongest argument there for not round-tripping a mounted config through PropertiesConfiguration at all, and it should shape whatever replaces it.
| log "ERROR: cannot protect ${REST_SERVER_CONF} before writing auth.admin_pa" | ||
| exit 1 | ||
| fi | ||
| if ! set_prop "auth.admin_pa" "${PASSWORD}" \ |
There was a problem hiding this comment.
ConfigTool set as a JVM command-line argument, so it is observable in process listings, /proc/<pid>/cmdline, container inspection, and command-line audit records during startup. The existing enabled bootstrap path already avoids this by using stdin. Please add a sensitive-value mode that reads from stdin or a file descriptor, and test that the launched Java argv never contains the password.
There was a problem hiding this comment.
Moved to #3133 and called out explicitly in it: not passing secrets as process arguments is one of the stated requirements for that work.
|
|
||
| # The gate returns before backend or plugin registration, so this performs | ||
| # validation only. | ||
| ./bin/init-store.sh |
There was a problem hiding this comment.
init-store.sh, whose wrapper unconditionally calls ensure_path_writable "${PLUGINS}" before Java. A hardened deployment with a read-only application/plugins mount therefore fails even though this path deliberately skips backend and plugin registration. Please invoke the validation directly through ConfigTool, or add a validation-only wrapper mode that bypasses plugin writability checks, with a read-only-plugins regression.
There was a problem hiding this comment.
The --validate-only flag is gone from this PR along with the rest of the entrypoint changes (#3133). What init-store.sh keeps here is only the exit-status propagation, so ensure_path_writable runs exactly as it does on master.
|
|
||
| echo "==> a read-only mount rejects an init-store env override" | ||
| new_install | ||
| chmod 444 "${INSTALL}/conf/rest-server.properties" |
There was a problem hiding this comment.
chmod 444 does not model the production container failure this test claims to cover. Both server Dockerfiles run the entrypoint as root, which can still truncate a regular mode-0444 file, while Actions runs this shell suite as a non-root host user; the test therefore never exercises a real read-only bind mount/EROFS path. Please add an image-level case with a genuinely read-only bind mount or filesystem and assert non-zero propagation, no server start, and no leaked scratch file.
There was a problem hiding this comment.
The entrypoint test suite moved to #3133. Agreed the chmod 444 case does not model the production failure, since both server Dockerfiles run the entrypoint as root — worth fixing when the suite comes back.
- migrate verified legacy auth volumes without old secrets - fail closed on REST and Gremlin auth mismatches - protect config rewrites and reject include flattening - read administrator passwords from standard input - verify read-only mounts in Docker CI
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The latest head still treats aligned authentication configuration as proof that a legacy administrator exists, so a no-auth volume later configured for authentication can skip bootstrap without a usable admin. Evidence: six independent exact-head lanes; test-docker-entrypoint.sh:419-428 creates only init_complete plus enable-auth.sh configuration and asserts init-store is not run; existing discussion_r3687911099 covers the blocker.
…asks for The Docker auth bootstrap and the ConfigTool it depends on grew into a second change sharing this branch, which made the diff hard to review as one idea. They move to a follow-up, leaving the option, the gate, the env mapping and the init-flag guard here. Review fixes carried over: - init-store.sh propagates InitStore's exit status. The trailing "Initialization finished." echo made the script exit 0 no matter what, so a refused configuration let the entrypoint start a server nobody could log in to. - The entrypoint normalizes HG_SERVER_INIT_STORE_ENABLED and rejects values HugeConfig would refuse, instead of recording an initialization that never happened. - The admin bootstrap check no longer requires auth.admin_pa, which nothing in the container writes. usePD and the hstore auth graph stay. - PASSWORD is documented as having no effect once init-store is skipped, and the entrypoint warns when both are set.
|
Rescoped this PR. It had grown to ~2,900 lines doing two unrelated things: the init-store gate #3118 asked for, and a rewrite of the Docker auth bootstrap built on a new What is left here is 496 lines across 7 files — the option, the gate, the env mapping, and the init-flag guard. Your 7 open threads are all on code that moved out. I have replied on each pointing at #3133 instead of resolving them, so they stay findable there. Two things worth flagging from re-reading it:
The admin-bootstrap check no longer requires 604 tests, 0 failures on JDK 11, checkstyle clean. Description updated to match. Not run: Mirrored to hugegraph#171. |
- Name the actual boolean converter in the entrypoint comment. "BooleanUtils" alone is ambiguous: commons-lang3 accepts 0 and 1, the commons-lang 2.x one reached through commons-configuration 1.x PropertyConverter does not, and the accepted spellings follow the latter. - Say that the init_complete guard applies to the environment variable, not to the property set directly in a mounted rest-server.properties. - Move InitStoreConfigTest into its own cmd group in UnitTestSuite instead of the core one. - Trim test comments closer to the density of the surrounding suite.
|
Follow-up push (5268091) after another review pass. No behaviour change — 604 tests, 0 failures, checkstyle clean.
Two things I would rather raise myself than have you find them:
One branch is effectively unreachable and untested. Also for ordering: #3132 (the Helm chart) injects |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The disabled-init path can expose a predictable administrator credential and can persist an incorrect initialization marker for mounted configurations. Evidence: exact-head static traces through InitStore, GraphManager, and docker-entrypoint.sh; 12/12 focused tests passed; all visible latest-head workflows passed.
…cord init by result Two findings from review of the disabled init-store path. The check now also requires an explicit non-empty auth.admin_pa. Without it a fresh built-in-auth deployment reaches GraphManager.initAdminUserIfNeeded() on the PD startup path, which creates the admin from that option's public 'pa' default while Docker PASSWORD is discarded on this path. The accepted-HStore test had no admin_pa, so it pinned exactly that case. The docker init marker is now written by init-store itself, and only when it initialized. The entrypoint could only decide from its own environment variable, so a config mounted with init_store.enabled=false was still recorded as initialized, and setting it back to true later skipped initialization for good. The marker path is passed in, so callers that do not want one are unaffected.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docker/README.md:178
- The README still says the entrypoint writes
docker/init_completeonly whenHG_SERVER_INIT_STORE_ENABLED=falseis set, but the entrypoint no longer writes the marker at all;InitStorewrites it only after a successful enabled run. As written, this incorrectly implies that disabling via a mountedrest-server.propertiesbehaves differently from disabling via the env var.
> With the variable set to `false`, the entrypoint deliberately never writes
> `docker/init_complete`, so a later re-enable is still able to initialize.
> Setting the property directly in a mounted `rest-server.properties` does not
> get that guard.
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:74
HG_SERVER_INIT_STORE_ENABLEDis mapped intorest-server.propertiesviaset_prop, butset_proponly detects existingkey=definitions. If a mountedrest-server.propertiesuses:as the separator (valid for Java properties), this will append a second logical definition, andHugeConfigwill load it as a list and fail (as the unit test documents). Consider teachingset_propto match both=and:separators (and ideally whitespace separators) when deciding whether to replace vs append.
INIT_STORE_ENABLED=$(printf '%s' "${HG_SERVER_INIT_STORE_ENABLED:-}" |
tr -d '[:space:]' | tr '[:upper:]' '[:lower:]')
case "${INIT_STORE_ENABLED}" in
"" | y | t | yes | on | true | n | f | no | off | false) ;;
*) log "ERROR: invalid HG_SERVER_INIT_STORE_ENABLED" \
"'${HG_SERVER_INIT_STORE_ENABLED}'"
exit 1 ;;
esac
[[ -n "${INIT_STORE_ENABLED}" ]] && \
set_prop "init_store.enabled" "${INIT_STORE_ENABLED}" "${REST_SERVER_CONF}"
…moved init-store writes the marker now, so a run disabled by a mounted property records nothing just as one disabled by the environment variable does. The text still described the entrypoint owning it, and still claimed the mounted case was unguarded, which the change removed.
|
Both Copilot points, one taken and one deferred. README (taken, ff1325a). Correct catch, and it was my own stale text. After the marker moved into
Leaving it here for three reasons. It is pre-existing and not specific to this option: the same That is the same reasoning behind moving |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Existing initialization markers can bypass the new disabled-path safety checks, and PD administrator creation failures are still swallowed. Evidence: six independent exact-head review lanes, static traces through docker-entrypoint.sh, InitStore, and GraphManager; InitStoreConfigTest passed 13/13; git diff --check passed; visible exact-head checks are green.
| # init-store writes the marker itself, and only if it initialized. Deciding | ||
| # here would mean guessing from the environment variable, which says | ||
| # nothing about a config mounted with the property already set. | ||
| export HG_SERVER_INIT_COMPLETE_MARKER="${DOCKER_FOLDER}/${INIT_FLAG_FILE}" |
There was a problem hiding this comment.
docker/init_complete marker is present. Previous releases wrote that marker after initialization, and this guard skips init-store.sh entirely; after an upgrade or configuration change to init_store.enabled=false, built-in auth can therefore start without checking usePD, the HStore auth graph, or an explicit auth.admin_pa. That can leave the admin unreachable or allow the public default credential. Please run the disabled-path validation on every startup regardless of the legacy marker (or version/revalidate the marker), and add an upgrade regression with an existing marker and invalid built-in-auth configurations.
| LOG.warn("Skipping init-store: '{}' is false in '{}'. Local " + | ||
| "backend and admin initialization are not performed.", | ||
| ServerOptions.INIT_STORE_ENABLED.name(), restConf); | ||
| checkAdminBootstrapReachable(restServerConfig, restConf); |
There was a problem hiding this comment.
GraphManager.initAdminUserIfNeeded() catches every exception from metaManager.createUser() and initDefaultGraphSpace() and only logs it, so PD write, permission, or validation failures can still let the server start without a usable administrator. Please ignore only the explicit already-exists case and propagate other bootstrap failures, with regressions for both an existing admin and a non-duplicate creation failure.
Fixes #3118
Kept in sync with hugegraph#171
Problem
InitStorealways performs local initialization: it scansconf/graphs,initializes non-HStore backends, and bootstraps the built-in admin account.
That is correct for standalone and tarball deployments, but a PD/HStore
deployment already owns its metadata, and its Kubernetes pods cannot rely on
the container-local
docker/init_completeflag surviving a restart.Solution
A dedicated
init_store.enabledoption, defaulting totrue, so existingstandalone and tarball configurations keep the current path unchanged.
Distributed deployments set it to
false— orHG_SERVER_INIT_STORE_ENABLED=falsein the server container — to skip backend and plugin registration, the graph
initialization scan, and local admin initialization.
#3118 proposed reusing
graph.load_from_local_config. A dedicated option wasused instead: that flag governs whether
GraphManagerloads graphs from./conf/graphs, which is a different question from whether init-store shouldrun, and overloading it would have coupled two unrelated decisions.
Scope
In scope
ServerOptionsinit_store.enabled,Boolean, defaulttrueInitStorebin/init-store.shInitStore's exit statusdocker-entrypoint.shHG_SERVER_INIT_STORE_ENABLEDonto the property, validates it, hands the marker path to Javadocker/README.mdInitStoreConfigTest, 13 casesBehaviour, by deployment shape
trueusePD=true+ HStore auth graph + explicitauth.admin_paauth.admin_paon the PD pathDeliberately out of scope
ConfigTool— split to [Bug] Docker entrypoint auth bootstrap is unsafe for mounted and upgraded configs #3133. Seven reviewthreads on this PR target that code and are answered there.
cannot implement the Java-properties grammar
HugeConfiguses. Solved hereby having
init-storeown the marker instead; a general config-reading CLIbelongs to [Bug] Docker entrypoint auth bootstrap is unsafe for mounted and upgraded configs #3133.
run-native-runtime-smoke-test.sh,start-server.shandtest-start-hugegraph.shall treat any non-zero status frombin/init-store.shas fatal.auth.admin_paapplies only whenthe account is first created.
graph.load_from_local_config,GraphManagerbehaviour, Helm job structure,shipped Compose
usePDdefaults.Downstream
#3132 (Helm chart) injects
HG_SERVER_INIT_STORE_ENABLED=falseand must landafter this PR. It also has to write
auth.admin_pa, which it does not today —commented there.
Fail-closed check
With the gate off and the built-in authenticator selected, the admin is created
on the PD startup path by
GraphManager.initAdminUserIfNeeded(conf.get(ADMIN_PA)).Only an HStore auth graph reads that account back, and
ADMIN_PAdefaults tothe public value
pa. DockerPASSWORDnever reaches this path —init-storereads it from stdin and the disabled path returns first.
So the disabled path refuses unless
usePD=true, the auth graph uses HStore,and
auth.admin_pais explicitly set and non-empty. Remote auth and customauthenticators are exempt. Refusing is what keeps a deployment from coming up
with a publicly known administrator credential.
bin/init-store.shpropagates the exit status for this to mean anything: itstrailing
echo "Initialization finished."previously made the script exit0regardless, so a refusal was swallowed and the entrypoint started the server
anyway.
Completion marker
init-storewritesdocker/init_completeitself, and only after it hasinitialized. The entrypoint supplies the path and no longer decides. Deciding
in shell meant reading the environment variable, which says nothing about a
config mounted with
init_store.enabled=falsealready set — that was recordedas initialized, and setting it back to
truelater skipped initializationpermanently. Nothing is written when no caller supplies a path, so the tarball
path is unaffected.
Validation
JDK 11:
Tests run: 605, Failures: 0, Errors: 0, Skipped: 1(theskip is pre-existing). Checkstyle clean in all modules.
bash -non both changed scripts.echo "$PASSWORD" | ./bin/init-store.shunder
set -euo pipefail.false/FALSE/" false"/No/OFF/true/t/YES,unset, and the rejected
0/1/disabled. The accepted set is exactly whatcommons-lang 2.x
BooleanUtilstakes, reached through commons-configuration1.x
PropertyConverter— note that is not commons-lang3, which would alsoaccept
0and1.Not covered: a successful enabled run writing the marker needs a live backend.
No end-to-end container build was run — entrypoint changes are verified by
extracted-script simulation and code reading.
shellcheckwas not available.